security: drop symlinks from Hosting deploy upload list#10478
security: drop symlinks from Hosting deploy upload list#10478wecbaiyk-blip wants to merge 1 commit intofirebase:mainfrom
Conversation
`src/listFiles.ts` enumerates the files that `firebase deploy --only
hosting` uploads to the Firebase Hosting CDN. It used `glob({ follow:
true })`, and even with `follow: false` glob still returns symlink-to-
file entries. The Uploader then `fs.readFile`s each entry, which
follows symlinks at the OS layer, so a symlink under the public dir
ends up uploading the bytes of its target.
The exposure is largest for CI workflows that extract attacker-supplied
tarballs into the public dir before invoking `firebase deploy` (the
classic PR-preview deploy pattern). A tarball entry like
`public/leak -> /proc/self/environ`, `public/leak -> ~/.config/gcloud/
application_default_credentials.json`, or `public/leak ->
~/.ssh/id_rsa` would otherwise end up published on a Firebase-hosted
public URL accessible to the world.
This change:
- Sets `follow: false` so `glob` does not descend into symlinked
directories.
- Adds an explicit `lstatSync`-based filter that drops any entry whose
`lstat` reports `isSymbolicLink()`. This is the choke point that
catches symlink-to-file entries `glob` still returns under
`follow: false`.
- Logs a `debug`-level entry when a symlink is dropped, so users who
legitimately rely on symlinks can find out what was skipped.
- Documents the threat model in JSDoc on `listFiles`.
Tests are added in `src/listFiles.spec.ts` covering top-level and
nested symlink-to-file rejection, plus the symlinked-directory case.
The existing fixture-based tests are unchanged.
Note: this complements the in-flight `fsAsync.readdirRecursive`
default-flip (PR firebase#10477) but does NOT overlap with it. `fsAsync` is
used by Cloud Functions deploy and App Hosting deploy; `listFiles` is
used by Hosting deploy. Both paths need symlink protection.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances security in listFiles by explicitly excluding symbolic links from the deployment list, preventing potential data leaks of files outside the source tree. It also introduces comprehensive unit tests for symlink handling. Feedback includes recommendations to use the node: prefix for built-in module imports, improving the robustness of test cleanup hooks with truthy checks, and ensuring consistent use of filesystem utilities.
| import * as crypto from "crypto"; | ||
| import * as fs from "fs"; | ||
| import * as os from "os"; | ||
| import * as path from "path"; | ||
| import { rmSync } from "node:fs"; |
There was a problem hiding this comment.
Inconsistent use of built-in module imports. It is recommended to use the node: prefix for all built-in modules for clarity and to follow modern Node.js conventions. Additionally, mixing namespace imports (import * as fs) with named imports (import { rmSync }) for the same module is redundant when the named export is available on the namespace object.
| import * as crypto from "crypto"; | |
| import * as fs from "fs"; | |
| import * as os from "os"; | |
| import * as path from "path"; | |
| import { rmSync } from "node:fs"; | |
| import * as crypto from "node:crypto"; | |
| import * as fs from "node:fs"; | |
| import * as os from "node:os"; | |
| import * as path from "node:path"; |
| after(() => { | ||
| rmSync(tmpRoot, { recursive: true, force: true }); | ||
| rmSync(outsideTarget, { force: true }); | ||
| }); |
There was a problem hiding this comment.
The after hook should safely handle cases where tmpRoot or outsideTarget might be empty strings (e.g., if the before hook fails before they are assigned). Calling rmSync("") can throw an error, which might obscure the actual failure in the test report. Adding truthy checks makes the cleanup more robust.
after(() => {
if (tmpRoot) {
fs.rmSync(tmpRoot, { recursive: true, force: true });
}
if (outsideTarget) {
fs.rmSync(outsideTarget, { force: true });
}
});| } catch { | ||
| /* ignore */ | ||
| } | ||
| rmSync(outsideDir, { recursive: true, force: true }); |
There was a problem hiding this comment.
Use fs.rmSync for consistency with the namespace import recommended above.
| rmSync(outsideDir, { recursive: true, force: true }); | |
| fs.rmSync(outsideDir, { recursive: true, force: true }); |
2 participants
Summary
src/listFiles.tsenumerates the files thatfirebase deploy --only hosting(andfirebase hosting:channel:deploy) uploads to the Firebase Hosting CDN. It usedglob({ follow: true }), and even withfollow: falseglobstill returns symlink-to-file entries becausefollowonly controls whether symlinked directories are descended into.Uploaderthen callsfs.readFile*on each entry, which follows symlinks at the OS layer, so a symlink under the public dir ends up uploading the bytes of its target.This PR sets
follow: falseand adds an explicitlstatSyncfilter that drops any entry whoselstatreportsisSymbolicLink().Threat model
The largest exposure is CI workflows that extract attacker-supplied tarballs into the public dir before invoking
firebase deploy(the classic PR-preview deploy pattern; see GitHub Security Lab on pwn requests). A tarball entry likepublic/leak -> /proc/self/environpublic/leak -> ~/.config/gcloud/application_default_credentials.jsonpublic/leak -> ~/.ssh/id_rsapublic/leak -> /run/secrets/<anything>would otherwise end up uploaded under a path the attacker chose, on a Firebase-hosted public URL accessible to the world.
What this PR does
src/listFiles.ts:follow: true->follow: falsesoglobdoes not descend into symlinked directories.lstatSync-based filter that drops any entry whoselstatreportsisSymbolicLink(). This catches the symlink-to-file entriesglobstill returns underfollow: false.debug-level entry when a symlink is dropped so users who legitimately rely on symlinks can see what was skipped.lstat-vs-statrationale in JSDoc.src/listFiles.spec.ts:Backward compatibility
Behavior changes only for users whose Hosting public directory contains symlinks they want followed during deploy. That population is small; the typical
public/,dist/,build/directory contains regular files only. Users who need symlink-following can bypasslistFilesby copying the dereferenced files into a real directory before deploy (e.g.cp -L), which is also the more predictable thing to do for a CDN upload.Why this is a separate PR from #10477
PR #10477 flips the default of
fsAsync.readdirRecursive. That function is used by Cloud Functions deploy and App Hosting deploy. Hosting deploy useslistFiles, which is a different module backed byglob. Both paths need symlink protection; the two fixes don't overlap.Testing
npm run test:compile && npm test -- --grep listFiles— three new tests pass, existing fixture-based tests unchanged.